TOOL_STRATEGY_RULES conflates safety policy with tool routing; split it and delete routing in favor of skill/schema-carried routing - #1822

Code Review Summary

This PR does exactly what issue #1820 asked: it partitions TOOL_STRATEGY_RULES by ADR-064 channel ownership rather than trimming it in place. I verified each of the issue's per-line disposition items against the actual diff (not the summary), confirmed no dangling references to deleted content remain anywhere in the codebase, confirmed update_node genuinely has no node_type parameter (so its exclusion from the schema-guidance test is correct, not an oversight), and ran the full nodespace-agent test suite (317 passed) plus clippy (clean). This is a net positive and a well-scoped, atomic change. One finding is important enough to flag clearly but does not block merge given how the PR itself frames it.

Requirements Check (issue #1820)

• ✅ Lines 37–40, 44 (safety invariants) retained verbatim as TOOL_STRATEGY_RULES's entire remaining content.
• ✅ Line 36 (node_type argument shape) moved to search_nodes, create_node, resolve_query parameter descriptions in tools.rs. Verified update_node has no node_type param (fn def_update_node, tools.rs:497-520 — only id, content, properties), so its exclusion is factually correct, not a gap.
• ✅ Lines 50–53, 56–57 (tool-usage reference) moved into the owning tools' own descriptions: search_nodes filter semantics were already covered pre-PR by that tool's existing description (verified — not duplicated by this PR, correctly left alone); search_semantic's markdown-shortcut rule added to its description; create_relationship's relationship_type strengthened to prefer schema-defined names.
• ✅ Line 41/55 (create_node → search_skills sequencing, schema_metadata type-mapping): confirmed already present and untouched in the "Node Creation" skill's instructions (skill_pipeline.rs:301-324, e.g. "CALL create_node NOW... search_skills was called", "TYPE MAPPING FROM schema_metadata"). Correctly identified as pre-existing coverage, nothing needed to move.
• ✅ Line 42 (create_schema skips search_skills): structurally true — Schema Creation's tool_whitelist (skill_pipeline.rs:332) doesn't include search_skills, and create_schema has its own tool description. No prose needed.
• ✅ Line 48 (update_node search-first) and Line 54 (resolve_query → search_nodes → update_node indirect-update chain): both now live in Graph Editing's skill instructions. FIND THEN UPDATE (pre-existing, unchanged) plus the new INDIRECT TARGET rule (skill_pipeline.rs:127) together cover this, and resolve_query was added to Graph Editing's tool_whitelist (skill_pipeline.rs:346) so the tool is actually reachable from that skill — it previously was not (confirmed: before this PR, resolve_query existed only as a standalone tool description with no skill routing to it, exactly matching the issue's stated gap).
• ✅ TOOL_STRATEGY_RULES contains only safety-invariant policy when done — confirmed by reading the final constant and by the new negative-assertion test (agent_guidance.rs:71-90) that forbids the six deleted routing phrases from reappearing.
• ⚠️ "update_node and create_node both route through search_skills when it is expected to fire, verified with the real LocalAgentLoop" — partially met, see finding below. The new test uses the real LocalAgentLoop dispatch code, but the "model" is MockEngine, a fixed queue of pre-scripted tool-call responses returned in order regardless of prompt content (confirmed by reading MockEngine::new/responses: Mutex<Vec<Vec<StreamingChunk>>> — there is no branching on the assembled prompt). This proves the plumbing correctly executes and sequences a search_skills → search_nodes → update_node chain if a model chooses to emit it. It does not and cannot prove a real model, given the shrunk prompt and Graph Editing's actual instructions, will choose that chain over going straight to search_nodes. That question is explicitly out of scope for a mock-model unit test and belongs to the agent-matrix eval — which the test's own doc comment acknowledges ("live-model adherence to it is a separate concern covered by the agent-matrix eval"). I'd call this criterion honestly met at the plumbing level and explicitly not met at the behavioral level, which the PR does not overclaim — it only claims plumbing support.
• ✅ No regression in the agent test suite — 317/317 passed locally, clippy clean.

Findings

Critical Issues

None.

Suggested Improvements

• 🟡 packages/agent/src/local_agent/agent_loop.rs:3393-3453 — The new regression test's doc comment is honest about its own limits ("proves the plumbing supports the routed path; live-model adherence... is a separate concern covered by the agent-matrix eval"), which is the right call and I want to affirm rather than dock for it. But the PR description and the issue's acceptance criterion both use the word "verified" for this exact claim, and a reader skimming the commit message alone (not the test's inline comment) could walk away believing this test is behavioral proof that Resolve whether skill instructions delivered as a tool result suppress action #1814's measured 100%-bypass regression is fixed. Given Resolve whether skill instructions delivered as a tool result suppress action #1814 is the whole reason this issue exists, I'd want the actual fix validated by re-running the same controlled confirmation setup that produced the original 100%-bypass finding, or at minimum an explicit note in the PR body (not just a code comment) that live-model verification is still pending against the agent-matrix eval. This is a documentation/expectation-setting issue, not a code defect — the plumbing change itself is correct and necessary regardless of the outcome of that eval.

• 🟢 packages/agent/src/skill_pipeline.rs:127 (INDIRECT TARGET rule) — This rule and the pre-existing FIND THEN UPDATE rule both fire for the same "ambiguous update target" situation and are adjacent in the same skill's rendered instructions, with FIND THEN UPDATE textually first. Since skill instructions are consumed by the same model whose prose-ordering sensitivity is exactly what ADR-064 is about, it's worth a low-cost sanity check (or a note in the PR/issue) that FIND THEN UPDATE's generic "search then update" doesn't get followed before the model reads far enough to see INDIRECT TARGET's "call resolve_query first" carve-out. Likely fine given both are short and in the same block, but this is precisely the class of ordering effect the parent ADR was written to catch, so it's worth being deliberate about rather than assuming order-independence.

Nitpicks

• Nit: packages/agent/src/agent_guidance.rs:35-45 — The doc comment above TOOL_STRATEGY_RULES references "issue TOOL_STRATEGY_RULES conflates safety policy with tool routing; split it and delete routing in favor of skill/schema-carried routing #1820 and its audit trail for the per-line disposition." CLAUDE.md's documentation standard says not to embed issue numbers in code comments (this rule is stated for nodespace-docs specifically, but the same rationale — comments outliving the ticketing system — applies here). Minor, and this is a code comment rather than an architecture doc, but worth dropping the number and describing the disposition inline or pointing at the ADR instead, since ADR-064 is the durable reference and the issue number will eventually be dead weight.
• Nit: packages/agent/src/local_agent/tools.rs:2279-2327 — The three new tests (node_type_params_bind_to_relevant_entity_types, search_semantic_description_covers_markdown_shortcut, create_relationship_type_prefers_schema_defined_names) are substring-existence checks against the tool description text, not semantic checks. That's appropriate for this kind of content (there's no better oracle for "does this prose say the right thing" short of an eval), but it does mean these tests will pass even if the guidance text were reworded into something confusing as long as the target substring survives — same class of shallow-but-acceptable test as the existing TOOL_STRATEGY_RULES string-membership tests this PR extends. Not asking for a change, just noting the coverage's actual strength for future reference.

Verification Notes (things I checked that turned out fine, recorded so they aren't re-litigated)

• Grepped the whole packages/ tree for "ALWAYS search_nodes first", "READ-THEN-WRITE", "SKILL COMPLETION", "NODE_TYPE COMES FROM", and schema_metadata routing chains outside skill_pipeline.rs — the only remaining hits are inside the new negative-assertion test (checking the strings are absent) and in packages/core/src/ops/skill_ops.rs / context_ops.rs, which is where schema_metadata is legitimately produced as a tool result, not duplicated guidance.
• seed_prompt_bodies_match_expected_bytes (prompt_assembler.rs) derives its expected value from format!("{}\n\n{}", SCHEMA_CREATION_RULES, TOOL_STRATEGY_RULES) at test time, so it can't drift into a stale fixture — no risk there despite the char-count changing.
• Char-count claim (6916 → 2004, 71%) is directionally correct; my own count of the raw source string literal (2240 vs. old 6874 chars) is close enough to the PR's numbers (likely measured on the assembled/escaped runtime string) that I have no concern about the claim being fabricated.
• all_tool_definitions().len() == 14 assertion is unaffected (no tools added/removed, only descriptions changed) and still passes.
• Ran cargo test -p nodespace-agent --lib: 317 passed, 0 failed. Ran cargo clippy -p nodespace-agent --lib --all-targets: clean, no warnings.

Recommendation: APPROVE

This is a well-executed, atomic implementation of ADR-064 against the single file the issue targeted, with genuine per-line verification rather than speculative deletion — the PR does the actual homework the issue's checklist demanded (I independently re-verified several of those disposition claims and they hold). The one real gap — mock-model plumbing test standing in for live-model behavioral proof of the #1814 regression fix — is self-disclosed in the test's own doc comment and is properly deferred to the agent-matrix eval rather than silently overclaimed in the code. It's worth tightening the PR-level language so that deferral is equally visible to someone who only reads the PR description, but that's a documentation clarity nit, not a reason to withhold approval on a change that is a clear net improvement to codebase health.

Addressed review feedback

Both findings from the pragmatic-code-review addressed in beb6198:

• 🟡 Important — "verified with the real LocalAgentLoop" test could read as behavioral proof of the Resolve whether skill instructions delivered as a tool result suppress action #1814 100%-bypass fix. Strengthened the test's doc comment to explicitly state it proves dispatch plumbing (tool order, argument passing, session state) via a scripted MockEngine that returns a fixed response queue regardless of prompt content — it cannot exercise routing choice. Live-model adherence remains open, tracked by the agent-matrix eval (scripts/eval/fixtures/agent-matrix.ts), which isn't part of test:all.
• 🟢 Suggestion — Graph Editing's FIND THEN UPDATE rule now explicitly cross-references the INDIRECT TARGET carve-out immediately below it, so their relationship is stated rather than left to prose-ordering inference (relevant given ADR-064 is specifically about prose-ordering/dilution effects).

Also addressed the nitpick: removed all "issue #1820" references from code comments (6 across 3 files), pointing at ADR-064 instead per the project's standing rule against embedding issue numbers in code/docs.

All changes re-verified: 317/317 nodespace-agent tests pass, clippy clean, full test:all + pre-push gate green.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: All changes are doc-comment clarifications and one cross-reference sentence added to existing skill prose — no logic changes, no new behavior, no architectural changes. The original review's only two findings (both non-critical) are fully addressed. Ready for merge at maintainer's discretion.